Skip to content

fix: process members in SCIM Groups POST through mapping table#62

Merged
TerrifiedBug merged 1 commit intomainfrom
fix/scim-group-member-sync
Mar 8, 2026
Merged

fix: process members in SCIM Groups POST through mapping table#62
TerrifiedBug merged 1 commit intomainfrom
fix/scim-group-member-sync

Conversation

@TerrifiedBug
Copy link
Copy Markdown
Owner

Summary

  • POST /Groups was creating the ScimGroup record but ignoring body.members entirely
  • When IdP sends a group with its member list during sync, members were never processed through the mapping table — users never got assigned to their mapped teams
  • Both the create and adopt paths now call applyGroupMembers() which looks up the group's mappings and creates TeamMember records for each member

Test plan

  • SCIM sync from IdP: verify users are assigned to mapped teams after group sync
  • Verify existing PATCH/PUT member operations still work
  • Verify unmapped groups are still no-ops (no team assignments)

POST /Groups was creating the ScimGroup record but completely ignoring
body.members. When the IdP sends a group with its member list during
sync, members were never processed through the mapping table, so users
never got assigned to their mapped teams.

Now both the create and adopt paths call applyGroupMembers which looks
up the group's mappings and creates TeamMember records for each member.
@github-actions github-actions bot added the fix label Mar 7, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 7, 2026

Greptile Summary

This PR fixes a gap in the SCIM POST /Groups handler where body.members was silently ignored — users sent by the IdP during a group sync never got assigned to their mapped teams. Both the group-create and group-adopt code paths now call the new applyGroupMembers helper, which loads the configured group→team mappings and calls the idempotent applyMappedMemberships to create or upgrade TeamMember records for each member.

Key observations:

  • The member value field is correctly resolved against user.id (confirmed: the SCIM Users service returns the database primary key as the SCIM id).
  • applyMappedMemberships is idempotent (find-before-create, never downgrades), so duplicate calls on IdP retry are safe.
  • The applyGroupMembers call in the create path runs in its own prisma.$transaction that is separate from the preceding scimGroup.create. If the member transaction fails after the group row has been committed, the route returns 400 and the scim.group_created audit entry is never written. The IdP's retry would hit the adopt path and correctly apply members, but with a scim.group_adopted audit action instead of scim.group_created, leaving the audit log with inconsistent provenance. Wrapping both operations (create + member apply) in a single interactive transaction would close this gap.

Confidence Score: 4/5

  • Safe to merge — the happy path is correct, member processing is idempotent, and IdP retries are handled gracefully. Audit-log inconsistency on partial failure is recoverable.
  • The core feature is correct: SCIM member value maps to internal user.id, applyMappedMemberships is idempotent, and unmapped groups remain no-ops. The notable concern is the non-atomic relationship between scimGroup.create and the separate prisma.$transaction in applyGroupMembers — a failure between them leaves the group row committed but returns 400, and the audit log records adopted instead of created on IdP retry. This is a correctness/audit issue rather than data loss or security risk. Because applyMappedMemberships is idempotent and most IdPs retry on 400, the operation is eventually consistent and safe, keeping the score at 4 rather than 5.
  • src/app/api/scim/v2/Groups/route.ts — review the ordering of scimGroup.create and applyGroupMembers to determine if wrapping both in a single transaction is necessary for your audit-log requirements.

Sequence Diagram

sequenceDiagram
    participant IdP
    participant POST /Groups
    participant DB
    participant applyGroupMembers
    participant loadGroupMappings
    participant applyMappedMemberships

    IdP->>POST /Groups: POST { displayName, members[] }
    POST /Groups->>DB: findUnique(ScimGroup, displayName)
    alt group already exists (adopt path)
        DB-->>POST /Groups: existing group
        POST /Groups->>DB: update externalId (if changed)
        POST /Groups->>applyGroupMembers: (displayName, members)
        applyGroupMembers->>loadGroupMappings: load all mappings from SystemSettings
        loadGroupMappings-->>applyGroupMembers: GroupMapping[]
        applyGroupMembers->>applyGroupMembers: getMappingsForGroup(mappings, displayName)
        alt no mappings for this group
            applyGroupMembers-->>POST /Groups: return (no-op)
        else mappings found
            applyGroupMembers->>DB: $transaction { for each member }
            loop each member.value
                DB-->>applyGroupMembers: user.findUnique(id)
                applyGroupMembers->>applyMappedMemberships: (tx, userId, groupMappings)
                applyMappedMemberships->>DB: findUnique(TeamMember)
                alt member not in team
                    applyMappedMemberships->>DB: teamMember.create
                else role upgrade needed
                    applyMappedMemberships->>DB: teamMember.update(role)
                end
            end
        end
        POST /Groups->>DB: writeAuditLog(scim.group_adopted)
        POST /Groups-->>IdP: 200 OK
    else group does not exist (create path)
        DB-->>POST /Groups: null
        POST /Groups->>DB: scimGroup.create(displayName, externalId)
        POST /Groups->>applyGroupMembers: (displayName, members)
        Note over POST /Groups,applyGroupMembers: ⚠ separate transaction — not atomic with create
        applyGroupMembers->>loadGroupMappings: load mappings
        loadGroupMappings-->>applyGroupMembers: GroupMapping[]
        applyGroupMembers->>DB: $transaction { for each member }
        POST /Groups->>DB: writeAuditLog(scim.group_created)
        POST /Groups-->>IdP: 201 Created
    end
Loading

Comments Outside Diff (1)

  1. src/app/api/scim/v2/Groups/route.ts, line 145-152 (link)

    Non-atomic group creation and member apply

    The scimGroup.create (line 145–150) commits immediately and is not wrapped with the subsequent applyGroupMembers call (line 152). If applyGroupMembers throws (e.g., transaction timeout, deadlock, user lookup failure), the group row is already persisted, the route returns 400, and the scim.group_created audit entry is never written (the catch block prevents it).

    On an IdP retry, the adopt path runs instead and successfully applies members, but records scim.group_adopted rather than scim.group_created in the audit log, making the provenance unclear.

    While the members are ultimately applied (via idempotent retry), the atomicity gap and audit-log inconsistency could be resolved by wrapping both operations in a single transaction:

    const group = await prisma.$transaction(async (tx) => {
      const created = await tx.scimGroup.create({
        data: { displayName, externalId: body.externalId ?? null },
      });
      await applyGroupMembers(displayName, body.members, tx); // pass tx
      return created;
    });
    await writeAuditLog({...});

    This ensures the group row and member assignments succeed together, or both roll back, keeping the audit log consistent.

Last reviewed commit: add1f65

@TerrifiedBug
Copy link
Copy Markdown
Owner Author

@greptile review

@TerrifiedBug TerrifiedBug merged commit 57d68a9 into main Mar 8, 2026
12 checks passed
@TerrifiedBug TerrifiedBug deleted the fix/scim-group-member-sync branch March 8, 2026 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant